SSH configuration resolver to more closely match OpenSSH #28
Merged
Conversation
Previously LoadIdentityFiles called ssh.ParsePrivateKey on every file
listed as IdentityFile in ssh_config and returned the parse error to the
caller on the first failure. That aborted the whole connection attempt
with a 'ssh: no key found' error bubbling up from inside the SSH dial.
The failure surfaced even when the SSH agent (e.g. yubikey-agent,
Secretive, 1Password, gpg-agent) was perfectly capable of covering the
requested identity. The most common trigger is a hardware-token setup
where the IdentityFile path points at a public-key-only file on disk
while the private half lives on the token and is accessed exclusively
through the agent. OpenSSH tolerates this and falls back to the agent;
pretty did not.
LoadIdentityFiles now:
- Silently skips IdentityFile entries whose files do not exist
(errors.Is(err, os.ErrNotExist)).
- Silently skips entries that contain only an authorized_keys-style
public key (detected via ssh.ParseAuthorizedKey), so hardware-token
identities are delegated to the SSH agent.
- Silently skips passphrase-protected keys
(ssh.PassphraseMissingError), since this process has no way to
prompt for a passphrase.
- Still returns a wrapped, path-qualified error for unreadable files
and for truly malformed private-key content so real
misconfigurations are not hidden.
Tests:
- Replace TestLoadIdentityFilesFailFast (which asserted the old
behaviour) with TestLoadIdentityFilesSkipsMissing.
- Add TestLoadIdentityFilesSkipsPublicKey for the hardware-token case.
- Add TestLoadIdentityFilesMixedSkipsUnusable to verify that a usable
private key alongside unusable entries still produces an auth method.
- Add TestLoadIdentityFilesSkipsPassphraseProtected.
- Add TestLoadIdentityFilesReturnsErrorForMalformedPrivateKey to pin
the one case that must still surface an error.
- Introduce a generateTestPublicKeyLine helper that synthesizes a
fresh ed25519 authorized_keys line at runtime, so no static key
material is checked into the tree.
pretty resolves ssh_config entries for every target and every jump
host, but the sshconfig.Context passed to the resolver left
Context.Exec unset. The ssh_config library silently evaluates every
'Match host X exec "..."' block as non-matching when Exec is nil, so
directives that pick a reachable jump host dynamically - common in
corporate configs, e.g.
Match host jump-alias exec "nc -zG 1 primary.example.net 22"
HostName primary.example.net
Match host jump-alias exec "nc -zG 1 secondary.example.net 22"
HostName secondary.example.net
never applied. Their HostName override was skipped, the alias stayed
literal, and the connection failed at DNS resolution with 'no such
host'.
Fix: wire an Exec callback through the resolver.
- Introduce shellMatchExec: runs the already-token-expanded command
via /bin/sh -c (cmd.exe /C on Windows), with stdin/stdout/stderr
detached and a 10s timeout cap so a misbehaving probe can't hang
the CLI.
- Expose it via a package-level matchExecFunc var so tests can stub
exec evaluation deterministically without shelling out. This
matches the existing stubbing pattern used for connectionFunc /
sessionFunc.
- Pass matchExecFunc as Context.Exec from SSHConfigResolver.resolve
so every host and jump-host resolution benefits without any caller
changes in cmd/.
Follow-on correctness fix surfaced by the above: once Match exec
starts actually firing, 'Match originalhost "jump-*" ProxyJump none'
patterns produce ProxyJump="none". OpenSSH treats that as an explicit
opt-out that cancels inherited ProxyJump rules, but pretty was parsing
it as a literal jump host named "none" and trying to dial it.
ResolveHost now drops 'none' before calling ParseProxyJump, and
ParseProxyJump also collapses to an empty slice whenever any component
equals "none" (case insensitive), guaranteeing no caller ever sees
the sentinel.
Tests:
- TestResolveHostMatchExecApplies: first matching exec probe wins
and its HostName is returned.
- TestResolveHostMatchExecAllFailKeepsAlias: when every probe fails
the alias is left unchanged.
- TestShellMatchExecSucceedsForTrue / FailsForFalse / EmptyCmd: pin
the default shell callback's semantics on POSIX.
- TestParseProxyJumpNoneDisablesJumps: covers 'none', 'None', 'NONE',
and surrounding whitespace.
- TestResolveHostProxyJumpNoneClearsJumps: end-to-end sanity check
that a specific-block ProxyJump=none beats a wildcard block and
results in zero jumps.
No changes required in ncode/ssh_config: it already parses Match exec
and expands %-tokens correctly; it just needs the caller to provide a
runtime Exec callback, which is this change.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #28 +/- ##
==========================================
+ Coverage 87.00% 89.88% +2.87%
==========================================
Files 18 18
Lines 1193 1226 +33
==========================================
+ Hits 1038 1102 +64
+ Misses 108 99 -9
+ Partials 47 25 -22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add focused tests for previously-uncovered branches in config.go: - Error propagation from loadConfig for non-ENOENT paths (ENOTDIR via intermediate regular file) and from both user and system config loads in LoadSSHConfig. - Resolver error propagation through getValue and getAllValues when the underlying library rejects an empty HostArg. - ResolveHost paths for HostSpec.Alias precedence, invalid Port values surfacing as a wrapped error, and fallback to currentUser() when no explicit fallback is provided. - LoadIdentityFiles wrapping non-ErrNotExist read errors with the offending path. - ParseProxyJump dropping empty/whitespace-only components between commas. Statement coverage for internal/sshConn/config.go: 84.61% -> 93.75%. Remaining gaps are platform-specific (Windows shell branch in shellMatchExec), environment-dependent (currentUser env fallbacks when user.Current() fails), or structurally unreachable under normal control flow.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request enhances the SSH configuration resolver to more closely match OpenSSH behavior, especially around handling
Match ... execblocks andProxyJumpdirectives. It introduces robust support for evaluatingMatch ... execpredicates, improves handling of identity files (including skipping unusable keys), and ensures that special values likeProxyJump noneare interpreted correctly. The test suite is expanded to cover these new behaviors.Key changes include:
Support for
Match ... execin SSH config resolutionshellMatchExecandmatchExecFunc) to evaluateMatch ... exec "<cmd>"blocks, with a timeout to avoid hanging on long-running commands. This allows conditional SSH config blocks to be respected, mirroring OpenSSH behavior. [1] [2]Match ... exechandling, including stubbed command execution for deterministic testing.Improved handling of
ProxyJumpdirectivesParseProxyJumpto treat the value "none" (case-insensitive) as an explicit opt-out, ensuring no proxy jumps are used when this is set, consistent with OpenSSH. [1] [2]ProxyJump nonedisables jumps as expected.Identity file loading robustness
LoadIdentityFilesto silently skip missing, public-key-only, or passphrase-protected identity files, allowing authentication to proceed via the SSH agent if needed.isAgentCoveredIdentityto detect when a file should be delegated to the agent.Test infrastructure improvements
These changes make the SSH configuration handling more robust and compatible with user expectations from OpenSSH, while expanding test coverage to ensure reliability.